Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SHS-6081: Arrow in Vertical Linked Cards have indented to the next line #1749

Merged
merged 2 commits into from
Feb 19, 2025
Merged

SHS-6081: Arrow in Vertical Linked Cards have indented to the next line #1749

merged 2 commits into from
Feb 19, 2025

Conversation

mariannuar
Copy link
Collaborator

@mariannuar mariannuar commented Feb 18, 2025 •
edited
Loading

Summary

Fix arrow indentation in vertical linked cards

Need Review By (Date)

asap

Urgency

high

Steps to Test

  1. Go to https://hs-colorful-gnc5hznyyk6qmrkwqkqqu9hx56kjfb38.tugboatqa.com/components/collections/collections-postcards/vertical-linked-cards or go to Components>Vertical Linked Cards and scroll down to Vertical Linked Cards with title and link in Colorful and Traditional
  2. Make sure there's a short text in the link (if there isn't edit the text) to confirm the arrow is align with the text and not indented to the next line. Just a note that it will be indented to the next line if the text is too large and the container small
  3. Now go to https://chemistry.stage.stanford.edu/outreach/chemical-education
  4. Confirm the empty headings/titles are not being displayed in the markup. There was a condition that "fixed" the issue in this ticket, BUT, that wasn't fixed at all and it was causing this new issue. So please, take a look at it.

Review Tasks

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Are PHP functions and variables in snake_case and not camelCase?
  • Does Drupal code follow Drupal Coding Standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided?

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

@ahughes3 ahughes3 temporarily deployed to Tugboat February 18, 2025 20:37 Destroyed
@mariannuar mariannuar requested a review from cienvaras February 18, 2025 20:52
@mariannuar mariannuar self-assigned this Feb 18, 2025
@ahughes3 ahughes3 temporarily deployed to Tugboat February 19, 2025 18:11 Destroyed
@mariannuar
Copy link
Collaborator Author

@cienvaras This is ready for review!

@ahughes3 ahughes3 self-requested a review February 19, 2025 19:47
@ahughes3
Copy link
Collaborator

@cienvaras this is one that fixes an issue we found during testing on Stage and is a blocker for this week's Prod release

cienvaras
cienvaras approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mariannuar LGTM!

@ahughes3 Ready for you

@cienvaras cienvaras assigned ahughes3 and unassigned mariannuar Feb 19, 2025
ahughes3
ahughes3 approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@ahughes3 ahughes3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I tested by comparing:

https://west-gnc5hznyyk6qmrkwqkqqu9hx56kjfb38.tugboatqa.com/students/explore-our-offerings-graduate-and-undergraduate-students

to

https://west-stage.stanford.edu/students/explore-our-offerings-graduate-and-undergraduate-students

since we have that environment on tugboat

@ahughes3 ahughes3 assigned joegl and unassigned ahughes3 Feb 19, 2025
@ahughes3 ahughes3 requested a review from joegl February 19, 2025 20:19
@ahughes3
Copy link
Collaborator

@joegl we've approved this PR and would like to get it reviewed/merged and tested again and approved on stage before we move forward with the release

@joegl
Copy link
Contributor

joegl commented Feb 19, 2025

Happy to to get this in but this whole condition set is a bit confusing to me and I don't understand exactly why it's necessary / what it's doing.

@joegl joegl merged commit 93ef917 into 11.7.1-release Feb 19, 2025
18 checks passed
@joegl joegl deleted the shs-6081-fix-arrow-in-vertical-linked-cards branch February 19, 2025 21:25
@cienvaras
Copy link
Collaborator

Thanks @joegl! The condition was originally added (and then fixed in this PR) to ensure that we get a NULL in the template when the title is empty. The extra processing to strip the tags was causing a non null value in the template that was messing with the conditionals, so now we're only applying it when the title has content.

@joegl
Copy link
Contributor

joegl commented Feb 20, 2025 •
edited
Loading

@cienvaras That makes sense, I'm just not understanding where the title loses content:

if (!empty($variables['title'])) {
    $title = is_array($variables['title']) ? $renderer->renderInIsolation($variables['title']) : $variables['title'];
    $variables['title'] = $title ? ['#markup' => trim(strip_tags($title))] : NULL;

The first line checks that the title is not empty. Second line renders it if it's an array. The only way it would be empty on line 3 then is if the renderInIsolation() method returns an empty value. Is that what's happening?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahughes3 ahughes3 ahughes3 approved these changes

@cienvaras cienvaras cienvaras approved these changes

@joegl joegl joegl approved these changes

Assignees

@joegl joegl

Labels
Needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mariannuar @ahughes3 @joegl @cienvaras